-
Notifications
You must be signed in to change notification settings - Fork 13.9k
compiletest: Migrate TestProps directive handling to a system of named handlers
#147955
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Some changes occurred in src/tools/compiletest cc @jieyouxu
|
|
Running a bunch of try jobs to check for unforeseen problems: @bors try jobs=x86_64-msvc-1,i686-msvc-1,x86_64-mingw-1,test-various,armhf-gnu,aarch64-apple,dist-i586-gnu-i586-i686-musl |
compiletest: Migrate `TestProps` directive handling to a system of named handlers try-job: x86_64-msvc-1 try-job: i686-msvc-1 try-job: x86_64-mingw-1 try-job: test-various try-job: armhf-gnu try-job: aarch64-apple try-job: dist-i586-gnu-i586-i686-musl
This comment has been minimized.
This comment has been minimized.
|
I'll review this |
|
☔ The latest upstream changes (presumably #148305) made this pull request unmergeable. Please resolve the merge conflicts. |
This step consists of two changes: - Renaming `self` to `props` - Inserting temporary comments to preserve line breaks This will make it easier to verify that the main migration commit preserves all of the lines being migrated.
Use `git diff --color-moved --color-moved-ws=ignore-all-space` (or similar) to verify that the directive-processing lines have been moved without changes.
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
One of the very silly things about directive processing in compiletest is that for each directive in the test file, we proceed to check it against dozens of different directive names in linear sequence, without any kind of indexed lookup, and without any early-exit after a known directive name is found (unless a panic occurs).
This PR is a big step away from that, by taking the
iter_directivesloop inTestProps::load_fromand making all of its directive processing dispatch to a hashtable of individual name-specific handlers instead.The handler system is set up in a way that should allow us to add capabilities or change the implementation as needed, without having to mass-modify the existing handlers (e.g. this is why the
handlerandmulti_handlerfunctions are used).This PR is focused on mass-migrating all of the
TestPropsdirective processing into handlers. Most of the resulting handlers could obviously be simplified further (e.g. by avoiding the redundant name checks that were needed in the pre-migration code), but I've avoided doing any such simplifications in this PR to keep its scope limited and make reviewing easier.The patches in this PR have been arranged so that the main migration can be inspected with
git diff --color-moved --color-moved-ws=ignore-all-spaceto verify that it moves all of the relevant lines intact, without modifying or discarding any of them.r? jieyouxu